refactor: rename "bulk" to more accurate "dynamically created" #1538
Conversation
metacosm
commented
Oct 12, 2022
metacosm
commented
- refactor: deleteBulkResource -> deleteTargetResource
- refactor: remove unneeded key parameter
- refactor: remove unneeded key parameter
- refactor: rename bulk to dynamically created to be more accurate
- refactor: remove now unneeded class
- refactor: more renaming
|
Kudos, SonarCloud Quality Gate passed! |
|
@metacosm I don't agree that dynamic is more accurate, actually misleading, synce every resource is is dynamically created by an operator nothing is "static". Bulk better describes that we are creating here a set of similar resources. |
|
Will check the other changes. |
| * @param context actual context | ||
| */ | ||
| void deleteBulkResource(P primary, R resource, String key, Context<P> context); | ||
| void deleteTargetResource(P primary, R resource, Context<P> context); |
There was a problem hiding this comment.
But not sure we want to remove the key. I'm more to more complete APIs, so that is an information that some might miss if providing a custom implementation. So this way it's more future proof, also integral part.
There was a problem hiding this comment.
OK, was debating this last point myself, actually…
| implements DependentResourceReconciler<R, P> { | ||
|
|
||
| private final BulkDependentResource<R, P> bulkDependentResource; | ||
| private final DynamicallyCreatedDependentResource<R, P> delegate; |
There was a problem hiding this comment.
As mentioned don't agree with term 'Dynamic' it's too generic notion. Why Bulk nicely identifies this feature.
There was a problem hiding this comment.
Bulk doesn't fit either, imo. It kinda worked when the solution was indexed-based but now it's more about dynamically (as opposed to statically known at build time) defined dependents.
There was a problem hiding this comment.
But the whole feature / funtionality is about to create resources in bulk. That is the only purpose, the only thing that changed is that the key is not an integer but a string.
There was a problem hiding this comment.
Bulk implies a large number and that's not really what the feature is about. This is about creating some resources that are determined at runtime, as opposed to the other dependents which are known statically at build-time. I agree that "dynamically created" isn't too appropriate but I don't think that "bulk" is, either…
| Map<String, R> actualResources, P primary, Context<P> context) { | ||
| actualResources.forEach((key, value) -> { | ||
| if (!expectedKeys.contains(key)) { | ||
| bulkDependentResource.deleteBulkResource(primary, value, key, context); | ||
| delegate.deleteTargetResource(primary, value, context); |
|
|
||
| // remove existing resources that are not needed anymore according to the primary state | ||
| deleteBulkResourcesIfRequired(desiredResources.keySet(), actualResources, primary, context); | ||
| deleteUnexpectedDynamicResources(desiredResources.keySet(), actualResources, primary, context); |
There was a problem hiding this comment.
It's not really unexpected. It's natural to have those. Maybe "surplus" ? but not sure if this word used in this context.
There was a problem hiding this comment.
What about deleteExtraResources?
That's not necessarily true anymore, though. It could be anything depending on what the primary spec says. Bulk was only appropriate when things were restricted to index-based similar resources but that's not the case anymore. |
Don't understand what do you mean, we still have |
|
We agreed to keep the bulk name because we couldn't find a better one and "dynamically created" is confusing. Replaced by #1544. |
